-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flav/input bar uses file api #5997
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo one comment 🤓 🔥
if (url && url.startsWith(FileResource.baseFileUrl(owner))) { | ||
// Use the provided URL if it is an internal file path. | ||
sourceUrl = url; | ||
} else if (isSupportedUploadableContentFragmentType(contentType)) { | ||
// Deprecated, for supported content types, create a file path and use its download URL. | ||
sourceUrl = fileAttachmentLocation({ | ||
workspaceId: owner.sId, | ||
conversationId: conversation.sId, | ||
messageId, | ||
contentFormat: "raw", | ||
}).downloadUrl; | ||
} else { | ||
// Otherwise, use the provided URL (e.g., a Slack thread). | ||
sourceUrl = url; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's different than previously?
Basically if we not in isSupportedUploadableContentFragmentType(contentType)
we set sourceUrl = url
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is that know the client passes an url that points toward our own infrastructure, if this is the case, we use it as the url, otherwise we fallback to the previous logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking here but unless I'm missing something this is equivalent to:
if (isSupportedUploadableContentFragmentType(contentType)) {
// Deprecated, for supported content types, create a file path and use its download URL.
sourceUrl = fileAttachmentLocation({
workspaceId: owner.sId,
conversationId: conversation.sId,
messageId,
contentFormat: "raw",
}).downloadUrl;
} else {
sourceUrl = url;
}
front/lib/resources/file_resource.ts
Outdated
@@ -83,6 +88,10 @@ export class FileResource extends BaseResource<FileModel> { | |||
}); | |||
} | |||
|
|||
static baseFileUrl(owner: LightWorkspaceType): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my previous comment this might be deadcode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary, we will rework this API to accept either a fileId or a content. This is an intermediary state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, super nice 🔥
onClose={() => { | ||
fileUploaderService.removeFile(blob.id); | ||
}} | ||
isLoading={blob.isUploading} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
* Use file upload API in the input bar * Stop uploading raw content fragment * ✨ * 👕 * 📝 * ✨ * ✂️ * ✂️ * ✂️ * ✂️ * :sparkles * ✨
* Use file upload API in the input bar * Stop uploading raw content fragment * ✨ * 👕 * 📝 * ✨ * ✂️ * ✂️ * ✂️ * ✂️ * :sparkles * ✨
Description
In #5977, we introduced a file API to manage all file-related tasks. This PR begins using that endpoint.
The key changes include:
Demo:
Risk
If we rollback, all conversations created during this time, won't support new messages.
Deploy Plan